Ruleset: hard deprecate use of the old property array format #890

jrfnl · 2025-03-18T06:48:21Z

Description

This format was (soft) deprecated in PHPCS 3.3.0 (June 2018) and it is expected that most rulesets will have been updated.

With the new "error handling" functionality in the Ruleset class in place (PR #857), the old format can now be hard deprecated to make users who haven't updated their rulesets yet, aware of the upcoming removal of support for the old array format.

Refs:

Suggested changelog entry

Added deprecation notices (hard deprecation) for:

The old array property setting format (via comma separated strings), which will be removed in PHPCS 4.0.

Related issues/external references

Closes #693

rodrigoprimo · 2025-03-21T19:15:31Z

tests/Core/Ruleset/PropertyTypeHandlingTest.php

+        $regex .= '(?:\./tests/Core/Ruleset/Fixtures/TestStandard/Sniffs/SetProperty/PropertyTypeHandlingSniff\.php|TestStandard\.SetProperty\.PropertyTypeHandling)';
+        $regex .= '"\.\R';
+        $regex .= 'Pass array values via <element \[key="\.\.\." \]value="\.\.\."> nodes instead\.\R';
+        $regex .= '){14}\R$`';


Is it intentional that the deprecation notice for each property is displayed twice? There are seven properties using the old array format in PropertyTypeHandlingTest.xml. So, I expected this regex group to repeat seven times, not fourteen.

On a quick check, it seems this happens only when a sniff path is passed to <rule ref=''>. It doesn't happen when a sniff code is passed.

When a sniff path is passed, $todo ends up with two entries for the same sniff (one with the path and another with the code), and then the code that adds the deprecation notice for a given property is executed twice:

PHP_CodeSniffer/src/Ruleset.php

Lines 1078 to 1100 in f2274bd

$todo = [$ref];

$parts = explode('.', $ref);

$partsCount = count($parts);

if ($partsCount <= 2

|| $partsCount > count(array_filter($parts))

|| in_array($ref, $newSniffs) === true

) {

// We are processing a standard, a category of sniffs or a relative path inclusion.

foreach ($newSniffs as $sniffFile) {

$parts = explode(DIRECTORY_SEPARATOR, $sniffFile);

if (count($parts) === 1 && DIRECTORY_SEPARATOR === '\\') {

// Path using forward slashes while running on Windows.

$parts = explode('/', $sniffFile);

}

$sniffName = array_pop($parts);

$sniffCategory = array_pop($parts);

array_pop($parts);

$sniffStandard = array_pop($parts);

$todo[] = $sniffStandard.'.'.$sniffCategory.'.'.substr($sniffName, 0, -9);

}

}

@rodrigoprimo You're 100% correct and I agree, it's not pretty to have those messages twice. I did consider filtering on sniff code, but ended up deciding against it for the following reasons:

If people include by file name, they may not be aware of the sniff code, so filtering out the file name messages would make it harder for them to identify what to fix in their ruleset.

If the sniff referenced by file doesn't follow the PHPCS naming conventions, the sniff code may not be descriptive at all.

That makes sense, and I'm okay with leaving the duplication for the reasons you mentioned.

We could maybe add a code comment documenting that the deprecated messages will be displayed twice when a sniff is included by file name.

Or check if it is easy to detect that the sniff was included by file name and then include the sniff path to the deprecation message and display it only once.

That being said, since those deprecation messages will be removed when 4.0 is released, I don't think those suggestions need to be addressed.

That being said, since those deprecation messages will be removed when 4.0 is released, I don't think those suggestions need to be addressed.

Exactly. This code should only be in the codebase for a short time anyway.

Having said that, the same issue will happen when the deprecation becomes an error in PHPCS 4.0, but hopefully people will never see that as they've fixed up their ruleset ahead of time (based on the deprecation).

Still, the same arguments against filtering out the duplicate messages would apply for 4.0.

Good point about the problem still existing in PHPCS 4.0 as the deprecations will become errors. I haven't considered it. I'm okay with merging this PR as is.

I just wondered if you considered the option below. Maybe it is not easy to implement so not worth it?

Or check if it is easy to detect that the sniff was included by file name and then include the sniff path to the deprecation message and display it only once.

I don't think this would be easy to implement (otherwise I'd have done it already). It would require some kind of keeping track of which deprecations have been registered already and for something which has been deprecated for 8 years now and will soon no longer be supported, I don't think it's worth the effort.

This format was (soft) deprecated in PHPCS 3.3.0 (June 2018) and it is expected that most rulesets will have been updated. With the new "error handling" functionality in the Ruleset class in place (PR 857), the old format can now be hard deprecated to make users who haven't updated their rulesets yet, aware of the upcoming removal of support for the old array format. Refs: * https://github.com/PHPCSStandards/PHP_CodeSniffer/releases/tag/3.3.0 * squizlabs/PHP_CodeSniffer 1665 Closes 693

jrfnl · 2025-03-23T08:59:26Z

Rebased without changes to get rid of imaginary merge conflict.

jrfnl added Type: meta Core Component: Ruleset labels Mar 18, 2025

jrfnl added this to the 3.x Last milestone Mar 18, 2025

rodrigoprimo reviewed Mar 21, 2025

View reviewed changes

github-actions bot added the Status: has merge conflict label Mar 22, 2025

jrfnl force-pushed the feature/693-sq1665-ruleset-deprecate-use-of-old-array-format branch from f2274bd to f63aa0c Compare March 23, 2025 08:59

github-actions bot removed the Status: has merge conflict label Mar 23, 2025

rodrigoprimo self-requested a review March 24, 2025 12:29

rodrigoprimo approved these changes Mar 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ruleset: hard deprecate use of the old property array format #890

Ruleset: hard deprecate use of the old property array format #890

jrfnl commented Mar 18, 2025

rodrigoprimo Mar 21, 2025

jrfnl Mar 21, 2025

rodrigoprimo Mar 24, 2025

rodrigoprimo Mar 24, 2025

jrfnl Mar 25, 2025

jrfnl Mar 25, 2025

rodrigoprimo Mar 25, 2025

jrfnl Mar 25, 2025

jrfnl commented Mar 23, 2025

	$todo = [$ref];

	$parts = explode('.', $ref);
	$partsCount = count($parts);
	if ($partsCount <= 2
	\|\| $partsCount > count(array_filter($parts))
	\|\| in_array($ref, $newSniffs) === true
	) {
	// We are processing a standard, a category of sniffs or a relative path inclusion.
	foreach ($newSniffs as $sniffFile) {
	$parts = explode(DIRECTORY_SEPARATOR, $sniffFile);
	if (count($parts) === 1 && DIRECTORY_SEPARATOR === '\\') {
	// Path using forward slashes while running on Windows.
	$parts = explode('/', $sniffFile);
	}

	$sniffName = array_pop($parts);
	$sniffCategory = array_pop($parts);
	array_pop($parts);
	$sniffStandard = array_pop($parts);
	$todo[] = $sniffStandard.'.'.$sniffCategory.'.'.substr($sniffName, 0, -9);
	}
	}

Ruleset: hard deprecate use of the old property array format #890

Are you sure you want to change the base?

Ruleset: hard deprecate use of the old property array format #890

Conversation

jrfnl commented Mar 18, 2025

Description

Suggested changelog entry

Related issues/external references

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl commented Mar 23, 2025